Skip to content

Allow users to override react.js and transformer.js for assets and engine #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Feb 16, 2014

Conversation

jtmalinowski
Copy link
Collaborator

As in: #5
Docs & tests coming. Overriding is implemented through Sprockets.

@jtmalinowski
Copy link
Collaborator Author

I just noticed there is a problem with rails <= 4.0, we need to fit initializer after appending default paths, but before finisher_hook, it's weird that initialize "...", :after => :append_assets_path did not work as intended, still investigating this.

@jtmalinowski
Copy link
Collaborator Author

Got this to work, but a cleanup is needed yet. I'm not sure why I couldn't disable sprockets cache, even with

config.assets.cache_store = :null_store

So there simply is a example2.js.jsx, to be compiled with dropped-in JSXTransformer.

@jtmalinowski
Copy link
Collaborator Author

@zpao let me know if you're fine with this approach before I proceed

@zpao
Copy link
Member

zpao commented Sep 18, 2013

How would you feel about making it a bit easier to drop files in vendor? It looks like Ember allows this by actually having 2 directories inside their vendor (details...). That way you don't have to do anything different in your assets. I'm not sure where JSXTransformer.js would fit into with 2 dirs... Maybe something like this:

vendor/react/
    JSXTransformer.js
    development/
        react.js
    production/
        react.js (would require manual renaming from react.min.js when put here)

@jtmalinowski
Copy link
Collaborator Author

Funny how they did it, just a 2 lines fix, I should have this finished in a few days.

@jtmalinowski
Copy link
Collaborator Author

Dirty, but it does the job.

# Make sure it can be found
app.assets.append_path(tmp_path)
# Allow overriding react files that are not based on environment
# e.g. /vendor/react/JSXTransformer.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: make sure this matches (missed assets)

@jtmalinowski
Copy link
Collaborator Author

I was away for a few days.
BTW, it's pretty important that we have a fix in JSXTransformer.js for execjs, that was introduced in 0.4.1, included info about this in docs.

@zpao
Copy link
Member

zpao commented Sep 26, 2013

I'm a bit confused why we need users to specify the version of react-source in their Gemfile. Could we still tie versions of the real-rails gem to a version of react-source? We could bump versions here as we update the dependency. If we do it right we could set the dependency to 0.4.x and that should be stable. When 0.5 comes out we change that to 0.5.x and bump the react-rails version accordingly (1.0 -> 1.1?)

@jtmalinowski
Copy link
Collaborator Author

If JSXTransformer interface stays unchanged, there's no need to tie them. I think adding something like react-source >= 0.4.1 would suffice. If we keep just s.add_dependency 'react-source' without any version, then bundler will take the newest one. So there's no need to make users add this declaration, but applications should define at least minor version of each gem they use (well, Semver says minor is backwards-compatible, but that's another topic) - and this is why it's recommended to define react-source version explicitly.

0.4 ~ 1.0
0.5 ~ 1.1

So you're just hiding the fact that react-rails and react-source versions are tied together...

@jtmalinowski
Copy link
Collaborator Author

OK, I got this rebased against current master - this is all as we discussed on IRC. Once we're ok here - I'll probably want to do some squashing.

@jtmalinowski
Copy link
Collaborator Author

All finished as discussed on IRC - rebased again to be up to date with master. I will be able to add units tests for #15 once this is merged.

@jtmalinowski
Copy link
Collaborator Author

any updates here? @zpao

@zpao
Copy link
Member

zpao commented Dec 20, 2013

Sorry for the delay! Things have been a little bit hectic lately and this project is lowest on the totem pole :(

I'm pushing out 0.5.2.0 and 0.8.0.0 right now, but if you rebase on top of those, I'll push the green button and merge it in, then we can go from there.

@jtmalinowski
Copy link
Collaborator Author

Sure thing, we could get back to our talk about streamlining this a bit if more PRs were to come.

```

Starting with `0.4.1` there is a fix, which alows execjs to transforms jsx files, so if you need a version lower than
`0.4.1` you will have to replace JSXTransformer.js, see below on how to do it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just drop this since it's old news.

@jtmalinowski
Copy link
Collaborator Author

This PR is so stretched in time I actually started forgetting how and what should be done. So AFAIR - react-source is to be specified in gemspec so adding it to Gemfile would achieve nothing. We are allowing to drop-in your chosen version of react into /vendor/assets/react instead.

@zpao
Copy link
Member

zpao commented Jan 7, 2014

Ok, cool. I couldn't remember exactly where we were but saw that part of the readme and got confused. If the readme is totally fine now then I'll just merge.

@jtmalinowski
Copy link
Collaborator Author

Totally fine.

@zpao
Copy link
Member

zpao commented Feb 10, 2014

#17 should fix the failing test (because coffeescript). Let's get one last rebase and then merge for real :)

While you're in there, lets set that version to 1.0.0pre or something, and get a couple more things in before we ship as 1.0. Sound ok?

zpao added a commit that referenced this pull request Feb 16, 2014
Allow users to override react.js and transformer.js for assets and engine
@zpao zpao merged commit cd9867f into reactjs:master Feb 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants